fix: Replace custom language names list with standard Intl DisplayNames API #2332 #2371

ahtesham-quraish · 2025-08-12T08:04:18Z

Description

Replace custom language names list with standard Intl DisplayNames API #2332

Supporting information

Link to other information about the change, such as GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.

Testing instructions

Please provide detailed step-by-step instructions for manually testing this change.

Other information

Include anything else that will help reviewers and consumers understand the change.

Does this change depend on other changes elsewhere?
Any special concerns or limitations? For example: deprecations, migrations, security, or accessibility.

Best Practices Checklist

We're trying to move away from some deprecated patterns in this codebase. Please
check if your PR meets these recommendations before asking for a review:

Any new files are using TypeScript (.ts, .tsx).
Deprecated propTypes, defaultProps, and injectIntl patterns are not used in any new or modified code.
Tests should use the helpers in src/testUtils.tsx (specifically initializeMocks)
Do not add new fields to the Redux state/store. Use React Context to share state among multiple components.
Use React Query to load data from REST APIs. See any apiHooks.ts in this repo for examples.
All new i18n messages in messages.ts files have a description for translators to use.
Imports avoid using ../. To import from parent folders, use @src, e.g. import { initializeMocks } from '@src/testUtils'; instead of from '../../../../testUtils'

codecov · 2025-08-12T08:18:53Z

Codecov Report

❌ Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 94.49%. Comparing base (0e1550a) to head (9f4da76).
⚠️ Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
...l/components/TranscriptWidget/LanguageSelector.jsx	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2371      +/-   ##
==========================================
+ Coverage   94.44%   94.49%   +0.05%     
==========================================
  Files        1169     1169              
  Lines       25102    25149      +47     
  Branches     5473     5492      +19     
==========================================
+ Hits        23708    23765      +57     
+ Misses       1328     1320       -8     
+ Partials       66       64       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…es API openedx#2332

bradenmacdonald

Thanks for this! Could you please finish updating the PR description? I was hoping you could explain some of your decisions a bit, like why you added the new iso-639-1 dependency.

bradenmacdonald · 2025-08-19T00:56:54Z

package.json

@@ -18,7 +18,7 @@
    "start": "fedx-scripts webpack-dev-server --progress",
    "start:with-theme": "paragon install-theme && npm start && npm install",
    "dev": "PUBLIC_PATH=/authoring/ MFE_CONFIG_API_URL='http://localhost:8000/api/mfe_config/v1' fedx-scripts webpack-dev-server --progress --host apps.local.openedx.io",
-    "test": "TZ=UTC fedx-scripts jest --coverage --passWithNoTests",
+    "test": "TZ=UTC fedx-scripts jest --passWithNoTests",


Suggested change

"test": "TZ=UTC fedx-scripts jest --passWithNoTests",

"test": "TZ=UTC fedx-scripts jest --coverage --passWithNoTests",

I think you committed this accidentally? I find the --coverage check here to be annoying as well, but we can't just change it without a dedicated PR and a bit more discussion.

bradenmacdonald · 2025-08-19T00:58:53Z

...s/VideoEditor/components/VideoSettingsModal/components/TranscriptWidget/LanguageSelector.jsx

 import { FileInput, fileInput } from '../../../../../../sharedComponents/FileInput';
+import { getLanguageName } from '../../../../../../data/constants/video';


Suggested change

import { getLanguageName } from '../../../../../../data/constants/video';

import { getLanguageName } from '@src/editors/data/constants/video';

This is simpler - see the best practices checklist in the PR description.

bradenmacdonald · 2025-08-19T00:59:52Z

...s/VideoEditor/components/VideoSettingsModal/components/TranscriptWidget/LanguageSelector.jsx

@@ -45,27 +45,27 @@ export const hooks = {
 };

 const LanguageSelector = ({
-  index, // For a unique id for the form control


Please leave this comment in place; I think it's helpful.

bradenmacdonald · 2025-08-19T01:00:15Z

...s/VideoEditor/components/VideoSettingsModal/components/TranscriptWidget/LanguageSelector.jsx

  language,
-  // Redux
-  openLanguages, // Only allow those languages not already associated with a transcript to be selected
+  openLanguages,


Please add back a comment to explain this parameter.

bradenmacdonald · 2025-08-19T01:02:58Z

...eoEditor/components/VideoSettingsModal/components/TranscriptWidget/LanguageSelector.test.jsx

@@ -46,13 +49,4 @@ describe('LanguageSelector', () => {
    render(<LanguageSelector {...props} language="" />);
    expect(screen.getByText('Select Language')).toBeInTheDocument();
  });
-
-  test('transcripts no Open Languages, all dropdown items should be disabled', () => {


Why is this test no longer needed?

bradenmacdonald · 2025-08-19T01:04:49Z

src/editors/data/constants/video.js

-  za: 'Zhuang',
-  zu: 'Zulu',
-});
+export const getLanguageName = (langCode, locales = ['en']) => {


If you can't convert this file to TypeScript yet, please move this function to a separate language-utils.ts file that is using TypeScript so that you can document the types properly. Any significant new code like this should have types defined. (And if you can't move it to a .ts file or convert this file to .ts you can always add JSDoc type comments.)

bradenmacdonald · 2025-08-19T01:08:45Z

src/editors/data/constants/video.js

+export const openLanguagesDataSet = ISO6391.getAllCodes().reduce((acc, code) => {
+  acc[code] = ISO6391.getName(code);
+  return acc;
+}, {});

 export const in8lTranscriptLanguages = (intl) => {


We don't need this in8lTranscriptLanguages code anymore. You can already use getLanguageName (the Intl.DisplayNames API) to get the name of a language in any other language.

bradenmacdonald · 2025-08-19T01:09:28Z

src/editors/data/constants/video.js


 export const in8lTranscriptLanguages = (intl) => {
  const messageLookup = {};
-  // for tests and non-internationlized setups, return en
+
+  // For tests and non-internationalized setups, return raw dataset


Node.js supports Intl.DisplayNames so it should work in tests too.

bradenmacdonald · 2025-08-19T01:09:44Z

src/editors/data/constants/video.test.js

+  });
+
+  it('returns language name from Intl.DisplayNames for given locale', () => {
+    displayNamesSpy = jest.spyOn(global.Intl, 'DisplayNames').mockImplementation(() => ({


I don't think you need to mock it?

bradenmacdonald · 2025-08-19T01:11:18Z

src/editors/data/constants/video.js

+      // Fallback to ISO6391 if Intl.DisplayNames fails
+    }
+  }
+  const isoName = ISO6391.getName(code);


If we include ISO6391 as a dependency, it undoes the bundle size savings of removing the language names from our code. Did you find a case where the Intl.DisplayNames API was not working, and this fallback is necesesary?

I think we should just fall back to (A) Intl.DisplayNames response but for English instead of the user's locale, and (B) if that doesn't work, just return the language code as the language name.

ahtesham-quraish force-pushed the ahtesham/#2332 branch from fd2e479 to 752cb2d Compare August 12, 2025 08:08

ahtesham-quraish force-pushed the ahtesham/#2332 branch from 752cb2d to dd4af33 Compare August 12, 2025 08:23

fix: Replace custom language names list with standard Intl DisplayNam…

9f4da76

…es API openedx#2332

ahtesham-quraish force-pushed the ahtesham/#2332 branch from dd4af33 to 9f4da76 Compare August 12, 2025 08:45

ahtesham-quraish requested a review from bradenmacdonald August 12, 2025 10:57

bradenmacdonald reviewed Aug 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Replace custom language names list with standard Intl DisplayNames API #2332 #2371

fix: Replace custom language names list with standard Intl DisplayNames API #2332 #2371

ahtesham-quraish commented Aug 12, 2025

Uh oh!

codecov bot commented Aug 12, 2025 •

edited

Loading

Uh oh!

bradenmacdonald left a comment

Uh oh!

bradenmacdonald Aug 19, 2025

Uh oh!

bradenmacdonald Aug 19, 2025

Uh oh!

bradenmacdonald Aug 19, 2025

Uh oh!

bradenmacdonald Aug 19, 2025

Uh oh!

bradenmacdonald Aug 19, 2025

Uh oh!

bradenmacdonald Aug 19, 2025

Uh oh!

bradenmacdonald Aug 19, 2025

Uh oh!

bradenmacdonald Aug 19, 2025

Uh oh!

bradenmacdonald Aug 19, 2025

Uh oh!

bradenmacdonald Aug 19, 2025

Uh oh!

Uh oh!

	"test": "TZ=UTC fedx-scripts jest --passWithNoTests",
	"test": "TZ=UTC fedx-scripts jest --coverage --passWithNoTests",

		import { FileInput, fileInput } from '../../../../../../sharedComponents/FileInput';
		import { getLanguageName } from '../../../../../../data/constants/video';

	import { getLanguageName } from '../../../../../../data/constants/video';
	import { getLanguageName } from '@src/editors/data/constants/video';

fix: Replace custom language names list with standard Intl DisplayNames API #2332 #2371

Are you sure you want to change the base?

fix: Replace custom language names list with standard Intl DisplayNames API #2332 #2371

Conversation

ahtesham-quraish commented Aug 12, 2025

Description

Supporting information

Testing instructions

Other information

Best Practices Checklist

Uh oh!

codecov bot commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bradenmacdonald left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Aug 12, 2025 •

edited

Loading